Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Improvements on whisper importer #704

Merged
merged 6 commits into from
Aug 10, 2017
Merged

Conversation

replay
Copy link
Contributor

@replay replay commented Aug 3, 2017

  • Fixes the initialization of the connection to the Cassandra index from the writer, previously this always tried to connect to localhost.
  • Adds auth support and insecure ssl support to the reader.
  • Better error handling in the reader.
  • A useful help message for the writer.
  • A status URL on the writer that can be used for health checks.

@replay replay requested review from Dieterbe and woodsaj August 3, 2017 11:44
@replay replay changed the title Whisper importer cassandra conn Improvements on whisper importer Aug 3, 2017
@@ -173,11 +188,18 @@ func processFromChan(files chan string, wg *sync.WaitGroup) {
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Content-Encoding", "gzip")

_, err = client.Do(req)
if len(*httpAuth) > 0 {
req.Header.Add("Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte(*httpAuth)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than manually adding the header, you can call

req.URL.User = url.UserPassword(user, pass)

But as you have the user/pass in a single string, what you are doing is probably easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thx, i'll leave it then

req.Header.Add("Authorization", "Basic "+base64.StdEncoding.EncodeToString([]byte(*httpAuth)))
}

resp, err := client.Do(req)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to drain the resp.Body in order for the connection to be re-used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -131,6 +171,7 @@ func main() {
server.Index.Init()

http.HandleFunc(*uriPath, server.chunksHandler)
http.HandleFunc("/status", server.statusHandler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is just for healthchecking, the convention is to use "/healthz"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@replay
Copy link
Contributor Author

replay commented Aug 3, 2017

@woodsaj that's all fixed

@tehlers320
Copy link

Just some feedback, i tested this on a large deploy it appears working.

Here is a validation that the metrics are stored, though i still do not see them in the graphs.


mt-store-cat -cassandra-keyspace metrictank -cassandra-addrs mtseed00:9042,mtseed01:9042,mtseed02:9042 '*' '1.1e961d29d7e5b5044743432fecca0d56_avg_28800' chunk-summary
# Looking for this metric:
1.1e961d29d7e5b5044743432fecca0d56_avg_28800 (base 1.1e961d29d7e5b5044743432fecca0d56 ) statsd.ip-10-1-1-1.c.numStats
# Keyspace "metrictank":
## Table metric_16
## Table metric_128
## Table metric_16384
1.1e961d29d7e5b5044743432fecca0d56_avg_28800_617 729d 7
1.1e961d29d7e5b5044743432fecca0d56_avg_28800_618 729d 84
1.1e961d29d7e5b5044743432fecca0d56_avg_28800_619 729d 84
1.1e961d29d7e5b5044743432fecca0d56_avg_28800_620 729d 64

@replay
Copy link
Contributor Author

replay commented Aug 3, 2017

thx for checking @tehlers320 . to make them show up you'll need to restart metrictank, because otherwise it will not re-read the index.

@tehlers320
Copy link

That's the part i don't understand, so my ID "1.fcc5e1772ffe25a18dc412e5a06afd43" is already there because i'm mirroring traffic from the old system.
If you search the index by metric name, "1.fcc5e1772ffe25a18dc412e5a06afd43" is the only id that exist. In this scenario should restart not be needed?

I gave it a restart anyways, no historical metrics. consolidateBy(average,sum,min,max) also does not bring up historical metrics.

Sorry to keep bothering you about this.

@replay
Copy link
Contributor Author

replay commented Aug 3, 2017

Do the retention configurations in MT and your imported whisper archives match? currently the importer does not modify the archives/retentions, so if these settings don't match some data might not show up depending on the differences.

@tehlers320
Copy link

ahh ugh, thats it . Thanks @replay

Well i really don't like my old retention schema darn!

@replay
Copy link
Contributor Author

replay commented Aug 3, 2017

I think sooner or later we'll probably need to add the ability to update retentions to the importer, because I bet you're not the only one with that problem.
Btw there's an update-ttl tool which you could use after you imported them with the schema that's currently applied in the whisper files: https://github.com/raintank/metrictank/blob/master/cmd/mt-update-ttl/main.go

@replay
Copy link
Contributor Author

replay commented Aug 3, 2017

@tehlers320 did you see the -read-archives parameter on the reader? depending on the exact mismatch between your retentions, this could be useful because it allows you to omit certain archives

@tehlers320
Copy link

@replay yes i tried reading only archive 5 but archive 5

This is what i inherited
10s:6h,60s:24h,5m:7d,15m:30d,60m:90d,8h:2y

8h:2y doesn't seem to work well with the chunk system i think.

@replay replay merged commit f08b23f into master Aug 10, 2017
@replay replay deleted the whisper_importer_cassandra_conn branch August 10, 2017 14:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants